Skip to content

feat: sigagg#426

Open
mskrzypkows wants to merge 61 commits into
mainfrom
sigagg
Open

feat: sigagg#426
mskrzypkows wants to merge 61 commits into
mainfrom
sigagg

Conversation

@mskrzypkows
Copy link
Copy Markdown
Contributor

No description provided.

@mskrzypkows mskrzypkows marked this pull request as ready for review May 21, 2026 12:12
Comment thread crates/core/src/sigagg.rs
pub type Result<T> = std::result::Result<T, SigAggError>;

/// Per-duty output: one aggregated [`SignedData`] per validator public key.
pub type AggSignedDataSet = HashMap<PubKey, Box<dyn SignedData>>;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the same type introduced in #430, I think after merging those two PRs we can make a shared type for this

Comment thread crates/core/src/sigagg.rs
Comment on lines +86 to +100
/// Callback invoked after a successful threshold aggregation for a duty.
pub type AggSub = Arc<
dyn Fn(&Duty, &AggSignedDataSet) -> Pin<Box<dyn Future<Output = Result<()>> + Send>>
+ Send
+ Sync
+ 'static,
>;

/// Verify callback — checks the aggregated signature against the beacon chain.
pub type VerifyFn = Arc<
dyn Fn(&PubKey, &dyn SignedData) -> Pin<Box<dyn Future<Output = Result<()>> + Send>>
+ Send
+ Sync
+ 'static,
>;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be great to replace this with async_trait

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about it, but what would it give us? A bit simpler definition of the callback which is not very complicated.

Comment thread crates/core/src/sigagg.rs Outdated
Comment thread crates/core/src/sigagg.rs Outdated
Comment thread crates/core/src/sigagg.rs Outdated
Comment thread crates/core/src/sigagg.rs Outdated
Comment thread crates/core/src/sigagg.rs Outdated
Comment thread crates/core/src/sigagg.rs Outdated
Comment thread crates/core/src/sigagg.rs Outdated
Comment thread crates/core/src/sigagg.rs Outdated
@mskrzypkows mskrzypkows requested a review from varex83 May 25, 2026 11:17
Comment thread crates/core/src/types.rs Outdated

/// Threshold BLS share index.
pub share_idx: u64,
pub share_idx: u8,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason to change this from u64 -> u8? share_idx is 1-based and max can be 256

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the reason: #426 (comment)
Hmm, why is it 1-based? That breaks possibility to keep it as u8 🤔

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or we can keep it in u8 but we need to remember to +1 every time we use it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this should be u64 instead of u8 to mach charon implementation:

pub type Index = u8;

So instead of changing to u8 I've should change to u64 the index.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So in Definition
https://github.com/NethermindEth/pluto/blob/main/crates/cluster/src/definition.rs#L72

/// Charon nodes in the cluster and their operators.
/// Max 256 operators.
pub operators: Vec<Operator>,

On node_idx()

/// Returns the node index for a given peer ID.
    pub fn node_idx(&self, pid: &PeerId) -> Result<NodeIdx, DefinitionError> {
        let peers = self.peers()?;

        for (i, peer) in peers.iter().enumerate() {
            if peer.id == *pid {
                return Ok(NodeIdx {
                    peer_idx: i,
                    share_idx: peer.share_idx(),
                });
            }
        }

        Err(DefinitionError::PeerNotFound { peer_id: *pid })
    }

That's call share_idx()

 /// Returns share index of this Peer. ShareIdx is 1-indexed while peer index
    /// is 0-indexed.
    pub fn share_idx(&self) -> usize {
        self.index.wrapping_add(1)
    }

So it will overflow if you keep it as u8. Since in dkg, we map the share_idx from definition to core type, this will fail

https://github.com/NethermindEth/pluto/pull/426/changes#diff-f8e65bf8b68144c18a39c840773e15d737ffe51d055e3605b5cb5d074a9a4beeR219

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also this should be appear in the test, the boundary check

Comment thread crates/core/Cargo.toml Outdated
Comment thread crates/core/src/sigagg.rs
BlstImpl.threshold_aggregate(&bls_sigs)
}
.map_err(|e| {
tracing::error!(parent: &span, error = %e, "threshold aggregate failed");
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the code we import debug from tracing, but here we use tracing::error, consider using only one approach around the codebase

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants